internal/restore/local.go (1)

285-303: npm dependencies might get deprioritized unexpectedly.

Here's what happens:

1. Line 285-288: deps is sorted and capped to 15
2. Line 290-295: npmRuntime deps are added (if not already in seen)
3. Line 296-302: npmDev deps are added (if not already in seen)

The issue is that if you have, say, a go.mod AND a package.json, the Go deps will fill up slots first, then npm runtime deps won't make it in because deps is already at 15.

I think you meant for npm runtime to have priority over other manifests' deps? If so, you might want to restructure this:

💡 Suggested approach

One way to fix this:

// Collect all deps first, then prioritize
sort.Strings(npmRuntime)
sort.Strings(npmDev)

// Add npm runtime first (highest priority)
for _, name := range npmRuntime {
    add(name)
}
// Then other deps
sort.Strings(deps)
for _, name := range deps {
    add(name)
}
// Then npm dev
for _, name := range npmDev {
    add(name)
}

// Now cap
if len(deps) > maxDeps {
    deps = deps[:maxDeps]
}

Or if the current behavior is intentional (npm deps as supplementary), just add a comment explaining the priority order.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/restore/local.go` around lines 285 - 303, The current logic sorts
and caps deps (deps, maxDeps) before attempting to add npmRuntime and npmDev,
which lets earlier manifest deps prefill the cap and deprioritize npm
runtime/dev; change the flow in the function that builds deps so npmRuntime is
added first (call add(name) for each name in sorted npmRuntime), then add the
existing deps list (sorted), then npmDev, and only after all prioritized
additions apply the cap (truncate deps to maxDeps); reference the existing
symbols deps, npmRuntime, npmDev, add, and maxDeps when making this change (or
alternatively document the current priority explicitly if the behavior is
intentional).

cmd/restore.go (2)

134-157: Consider reusing existing zip creation logic.

There's already a createZip function in internal/analyze/zip.go (see the relevant code snippet) that does essentially the same thing: git archive with directory-walk fallback. You might want to export that and reuse it here to avoid maintaining two copies.

That said, I get that you might want the restore-specific version to have slightly different behavior (like the 10MB file limit). If that's intentional, maybe add a quick comment explaining why this is separate from the analyze version.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 134 - 157, The restoreCreateZip function
duplicates logic already implemented by createZip in internal/analyze/zip.go;
either import and call that createZip (export it if needed) or, if
restoreCreateZip must keep restore-specific behavior (e.g., the 10MB file
limit), add a short comment above restoreCreateZip explaining the intentional
differences and why it cannot reuse createZip; if choosing to reuse, replace
restoreCreateZip body with a call to createZip(dir) and remove restoreWalkZip
duplication, ensuring any restore-specific limits are handled by parameters or a
thin wrapper around createZip.

---

176-204: Minor: restoreWalkZip silently swallows some errors.

Lines 177-179 and 180-183 return nil on errors, which means walk errors and filepath.Rel failures are silently ignored. This is probably intentional for robustness (you don't want one bad file to kill the whole zip), but worth noting.

Also, there's a small edge case: you skip hidden directories (line 185 via skipDirs) and check file size (line 190), but you don't explicitly skip hidden files in this function (unlike collectFiles in local.go which does at line 345). Not a big deal since git archive handles this in the happy path, and hidden files in the fallback path might actually be wanted.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 176 - 204, The filepath.Walk callback inside
restoreWalkZip is silently swallowing errors from the walk and filepath.Rel
calls and also doesn't skip hidden files; update the anonymous walk function
used by filepath.Walk so that it returns the encountered errors (or wraps them
with context) instead of returning nil for the errors from the incoming err
parameter and filepath.Rel, and add the same hidden-file check used by
collectFiles (i.e., skip files whose base name starts with '.' before
processing) so hidden files are treated consistently; refer to the anonymous
function passed to filepath.Walk, filepath.Rel, skipDirs and the info.IsDir/size
checks to locate where to change the returns and add the hidden-file guard.

internal/restore/types.go (1)

136-136: Static analysis: Consider using index-based iteration.

The linter flags that iterating over domains copies 128 bytes per iteration. You can use index-based iteration to avoid this:

🔧 Proposed fix

-	for _, d := range domains {
-		seen := make(map[string]struct{}, len(d.KeyFiles))
-		for _, f := range d.KeyFiles {
+	for i := range domains {
+		seen := make(map[string]struct{}, len(domains[i].KeyFiles))
+		for _, f := range domains[i].KeyFiles {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/restore/types.go` at line 136, The range loop iterates over domains
by value causing a ~128-byte copy each iteration; change the iteration to
index-based to avoid copying (replace the `for _, d := range domains {` loop
with an index loop and take a pointer or reference to the element, e.g. use `for
i := range domains { d := &domains[i] }`), and update any subsequent uses of `d`
in the loop to use the pointer/reference form (or the indexed value) so
semantics remain identical while eliminating the per-iteration copy.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/restore/types.go (1)

84-111: Deduplicate relationship/dependency lists before storing them.

DependsOn (Line 86) and ExternalDeps (Line 109) currently preserve duplicates from IR input. That can bloat rendered output and waste token budget. Consider deduping while preserving first-seen order.

♻️ Suggested patch

 	// Build domain → dependsOn map from DOMAIN_RELATES edges.
 	dependsOn := make(map[string][]string)
+	depSeen := make(map[string]map[string]struct{})
 	for _, rel := range ir.Graph.Relationships {
 		if rel.Type == "DOMAIN_RELATES" && rel.Source != "" && rel.Target != "" {
-			dependsOn[rel.Source] = append(dependsOn[rel.Source], rel.Target)
+			if depSeen[rel.Source] == nil {
+				depSeen[rel.Source] = make(map[string]struct{})
+			}
+			if _, ok := depSeen[rel.Source][rel.Target]; ok {
+				continue
+			}
+			depSeen[rel.Source][rel.Target] = struct{}{}
+			dependsOn[rel.Source] = append(dependsOn[rel.Source], rel.Target)
 		}
 	}
@@
-	var extDeps []string
+	var extDeps []string
+	extSeen := make(map[string]struct{})
 	for _, node := range ir.Graph.Nodes {
 		if node.Type == "ExternalDependency" && node.Name != "" {
+			if _, ok := extSeen[node.Name]; ok {
+				continue
+			}
+			extSeen[node.Name] = struct{}{}
 			extDeps = append(extDeps, node.Name)
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/restore/types.go` around lines 84 - 111, The DependsOn slices built
from ir.Graph.Relationships and the extDeps slice from ir.Graph.Nodes can
contain duplicates; iterate each source's dependency list and extDeps with a
small seen map to filter out duplicates while preserving first-seen order before
assigning to Domain.DependsOn and before returning/using extDeps; specifically
dedupe entries coming from ir.Graph.Relationships when you populate
dependsOn[rel.Source] and dedupe when building extDeps from nodes (use the
unique symbols dependsOn, Domain.DependsOn, extDeps, ir.Graph.Relationships,
ir.Graph.Nodes to locate where to insert the seen-map filtering).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cmd/restore.go (3)

148-152: Minor: stderr output inconsistency.

Line 149 sends git archive errors to os.Stderr directly, but the rest of the command uses cmd.ErrOrStderr() (e.g., line 90, 125). In tests where stderr is captured/redirected, git errors would leak to the real stderr.

Probably fine for now since git failures trigger the fallback anyway and aren't user-facing errors, but worth knowing if you ever want to test this path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 148 - 152, The git archive invocation sets
cmd.Stderr = os.Stderr which leaks errors to the real stderr; change it to use
the same helper as elsewhere by assigning cmd.Stderr = cmd.ErrOrStderr() (i.e.,
update the cmd variable created in the git archive block so it uses
cmd.ErrOrStderr() instead of os.Stderr to keep stderr capture consistent with
the other uses on lines referencing cmd.ErrOrStderr()).

---

83-84: Consider logging config load errors at debug level.

Right now, if config.Load() fails because of a corrupted config file (say, invalid JSON), the user just silently falls back to local mode with no indication. That's probably fine for "config doesn't exist" but could be confusing for "config exists but is broken."

Not a blocker — the current behavior is safe — but a debug-level log here could save someone 20 minutes of head-scratching someday.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 83 - 84, Capture and check the error returned
from config.Load() instead of discarding it (replace "cfg, _ := config.Load()"
with "cfg, err := config.Load()"), and if err != nil log the error at debug
level (e.g., logger.Debugf or the existing debug logging facility) with context
like "failed to load config" so corrupt-config cases are visible; keep the
subsequent hasAPIKey logic (uses cfg and hasAPIKey) unchanged.

---

131-132: Heads up: Description field will be empty in API mode but populated in local mode.

When you use FromSupermodelIR here (API mode), the Description field stays empty. But in local mode, BuildProjectGraph reads it from the README or package.json. Since the render template uses this field (see the template on line 45 and line 250), your rendered output will look different between the two modes.

For example:

• API mode: No description shown
• Local mode: Description appears below the project name

If this is intentional (API relies on other domain data instead), you're good. Just wanted to flag it in case you expected consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 131 - 132, The graph produced by
restore.FromSupermodelIR(ir, projectName) leaves the Description field empty in
API mode, causing inconsistent rendering versus BuildProjectGraph (local mode)
which populates Description from README/package.json; update the restore flow to
either populate graph.Description after calling FromSupermodelIR (e.g., copy
from ir.Description or lookup README/package.json for the project) or add a
fallback before rendering so templates use graph.Description || ir.Description
|| metadata README content; touch the code around FromSupermodelIR and the
rendering entrypoint to ensure Description is consistently set in API mode.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/restore/local.go (2)

72-86: Minor: truncation message says "chars" but counts runes.

Super small nit, but Line 83 says "first 3000 chars" while you're actually counting runes (which is the correct behavior for multi-byte characters like emoji or CJK). Consider updating the message to say "runes" or "characters" for accuracy, though this won't affect functionality.

✏️ Suggested tweak

 	if len(runes) > maxRunes {
-		content = string(runes[:maxRunes]) + "\n\n*(CLAUDE.md truncated — showing first 3000 chars)*"
+		content = string(runes[:maxRunes]) + "\n\n*(CLAUDE.md truncated — showing first 3000 characters)*"
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/restore/local.go` around lines 72 - 86, The truncation message in
ReadClaudeMD currently says "first 3000 chars" while the code slices by runes
(maxRunes); update the message string used when truncating (the literal appended
after string(runes[:maxRunes])) to accurately describe what is being counted
(e.g., "first 3000 runes" or "first 3000 characters") so it matches the behavior
controlled by maxRunes in ReadClaudeMD.

---

88-143: Dependency name collision risk with go.mod parsing.

Hey, quick heads up on Line 143 - you're extracting just the last segment of Go module paths. So github.com/user/zap and github.com/other/zap would both become just "zap". This is probably fine for a high-level summary, but worth being aware of if you ever need to disambiguate.

segs := strings.Split(mod, "/")
add(segs[len(segs)-1])  // "github.com/uber-go/zap" → "zap"

Not blocking - just something to keep in mind.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/restore/local.go` around lines 88 - 143, DetectExternalDeps
currently reduces Go module imports to only their last path segment
(segs[len(segs)-1]) which causes collisions like github.com/user/zap and
github.com/other/zap; update the Go parsing branch to add the full module path
(use mod) instead of only the final segment so the add function records the full
module identifier (refer to DetectExternalDeps, the local add closure, and the
mod/segs usage) — alternatively, if shorter names are desired, normalize to a
longer disambiguating slice (e.g., last two path segments) before calling add.

cmd/restore.go (1)

84-85: Config load errors are silently ignored.

Hey, just noting that cfg, _ := config.Load() ignores any errors from loading config. This means if there's a corrupted config file or permission issue, the user won't know why API mode isn't working - they'll just silently get local mode.

This might be intentional (treat config errors as "no config"), but consider at least logging a debug message for troubleshooting purposes.

💡 Optional: log config load errors

-	cfg, _ := config.Load()
+	cfg, cfgErr := config.Load()
+	if cfgErr != nil {
+		// Config errors are non-fatal; we'll fall back to local mode
+		fmt.Fprintf(cmd.ErrOrStderr(), "debug: config load failed (%v), using local mode\n", cfgErr)
+	}
 	hasAPIKey := cfg != nil && cfg.APIKey != ""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/restore.go` around lines 84 - 85, The code currently ignores errors from
config.Load() which can hide why API mode isn't enabled; change the call from
cfg, _ := config.Load() to capture the error (e.g., cfg, err := config.Load()),
and if err != nil log a debug or warning message (using the existing logger in
this package) before continuing so operators can see config load failures; keep
the existing hasAPIKey logic (hasAPIKey := cfg != nil && cfg.APIKey != "") so
behavior is unchanged, but ensure the error is surfaced via logging for
troubleshooting.